Skip to content

Add Python venv setup script and fix pyproject.toml#322

Open
krystophny wants to merge 1 commit intomainfrom
feature/venv-setup
Open

Add Python venv setup script and fix pyproject.toml#322
krystophny wants to merge 1 commit intomainfrom
feature/venv-setup

Conversation

@krystophny
Copy link
Copy Markdown
Member

Summary

  • Add setup-venv.sh script that creates .venv with all Python dependencies and builds pysimple
  • Add make venv / make venv-nopy targets
  • Fix requirements.txt: use minimum version constraints instead of exact pins (exact pins break on Python 3.14 due to missing prebuilt wheels)
  • Fix pyproject.toml: remove invalid [project.package-data] section rejected by scikit-build-core >= 0.12
  • Add .venv/ to .gitignore

Test plan

  • ./setup-venv.sh creates working venv with pysimple
  • source .venv/bin/activate && make test passes all tests (including previously-failing Python tests)
  • ./setup-venv.sh --no-pysimple works for deps-only install

@qodo-code-review
Copy link
Copy Markdown

ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan

Review Summary by Qodo

Add Python venv setup script and fix dependency constraints

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add setup-venv.sh script to automate Python venv creation with dependencies
• Add make venv and make venv-nopy targets for convenient environment setup
• Replace exact version pins with minimum constraints in requirements.txt
• Remove invalid [project.package-data] section from pyproject.toml
• Add .venv/ directory to .gitignore
Diagram
flowchart LR
  A["setup-venv.sh script"] -->|creates| B[".venv directory"]
  B -->|installs| C["Python dependencies"]
  C -->|builds| D["pysimple bindings"]
  E["Makefile targets"] -->|calls| A
  F["requirements.txt"] -->|minimum versions| C
  G["pyproject.toml"] -->|remove invalid section| H["valid config"]
Loading

Grey Divider

File Changes

1. setup-venv.sh ✨ Enhancement +65/-0

New venv setup automation script

• New bash script that creates and configures Python virtual environment
• Installs all dependencies from requirements.txt using pip
• Optionally builds pysimple Fortran-Python bindings via CMake
• Supports --no-pysimple flag for dependencies-only installation
• Includes comprehensive help documentation and error handling

setup-venv.sh


2. Makefile ✨ Enhancement +7/-1

Add venv setup make targets

• Add venv and venv-nopy targets to .PHONY declaration
• Implement venv target that calls ./setup-venv.sh
• Implement venv-nopy target that calls ./setup-venv.sh --no-pysimple

Makefile


3. requirements.txt 🐞 Bug fix +13/-12

Replace exact pins with minimum versions

• Replace exact version pins with minimum version constraints (e.g., ==2.2.6>=2.2)
• Update comment to explain minimum versions for Python compatibility
• Keep f90wrap==0.2.16 pinned due to API sensitivity
• Affects: numpy, scikit-build-core, pytest, pytest-cov, netCDF4, matplotlib, scipy, shapely

requirements.txt


View more (2)
4. pyproject.toml 🐞 Bug fix +0/-3

Remove invalid package-data section

• Remove invalid [project.package-data] section
• Section was rejected by scikit-build-core >= 0.12 as unknown key
• Maintains all other valid configuration sections

pyproject.toml


5. .gitignore ⚙️ Configuration changes +0/-0

Ignore Python virtual environment directory

• Add .venv/ directory to ignore list
• Prevents virtual environment from being committed to repository

.gitignore


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Mar 19, 2026

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 📐 Spec deviations (0)

Grey Divider


Action required

1. Stale CMake disables bindings 🐞 Bug ✓ Correctness
Description
setup-venv.sh only configures CMake when build/build.ninja is missing, so an existing build/
configured with ENABLE_PYTHON_INTERFACE=OFF can be reused and pysimple won’t be built correctly.
This can lead to pip install -e . producing a package that fails to import because the python/
subdirectory is skipped by CMake.
Code

setup-venv.sh[R49-58]

+    if [ ! -f "${SCRIPT_DIR}/build/build.ninja" ]; then
+        echo "  Configuring CMake ..."
+        cmake -S "$SCRIPT_DIR" -B"${SCRIPT_DIR}/build" -GNinja \
+            -DCMAKE_BUILD_TYPE=Release -DCMAKE_COLOR_DIAGNOSTICS=ON
+    fi
+    echo "  Building Fortran library ..."
+    cmake --build "${SCRIPT_DIR}/build" --config Release
+
+    echo "  Installing pysimple in editable mode ..."
+    pip install --no-build-isolation -e "$SCRIPT_DIR"
Evidence
The script’s “configure only if build.ninja is missing” logic can reuse a cached build/ that was
explicitly configured without Python bindings. This repo’s Makefile has a target that configures
build/ with ENABLE_PYTHON_INTERFACE=OFF, and the top-level CMakeLists.txt only adds the python/
subtree when ENABLE_PYTHON_INTERFACE is ON. Because scikit-build-core is configured to use build/ as
its build directory, pip’s editable install will also be impacted by the cached build settings.

setup-venv.sh[46-58]
Makefile[73-85]
CMakeLists.txt[498-500]
pyproject.toml[13-19]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`setup-venv.sh` reuses an existing `build/` directory if `build/build.ninja` exists, without ensuring `-DENABLE_PYTHON_INTERFACE=ON`. If a developer previously ran `make build-deterministic-nopy` (which configures `build/` with `ENABLE_PYTHON_INTERFACE=OFF`), the venv setup will reuse that cache and the Python bindings won’t be built/installed correctly.

### Issue Context
- `Makefile` has a `build-deterministic-nopy` target that configures `build/` with `-DENABLE_PYTHON_INTERFACE=OFF`.
- Top-level `CMakeLists.txt` only adds `python/` when `ENABLE_PYTHON_INTERFACE` is enabled.
- `pyproject.toml` sets scikit-build-core `build-dir = "build"`, so pip editable installs will interact with the same build directory.

### Fix Focus Areas
- setup-venv.sh[46-58]
- Makefile[73-85]
- pyproject.toml[13-19]

### Implementation direction
Do one of the following (preferred options first):
1. Use a dedicated build directory for the venv workflow (e.g. `build_venv/`), so it cannot conflict with `make build-deterministic-nopy`.
2. Always run an explicit reconfigure step when `BUILD_PYSIMPLE=1`, passing `-DENABLE_PYTHON_INTERFACE=ON` (and optionally `-DSIMPLE_ENABLE_PYTHON_TOOLS=ON`) even when `build/build.ninja` exists.
3. Detect a cached `CMakeCache.txt` with `ENABLE_PYTHON_INTERFACE:BOOL=OFF` and either error out with a clear message or automatically reconfigure/clean.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. venv-nopy not phony 🐞 Bug ⚙ Maintainability
Description
Makefile defines a venv-nopy target but does not include it in .PHONY, so a file named venv-nopy in
the repo root would prevent the recipe from running. This makes the new convenience target slightly
fragile.
Code

Makefile[17]

+.PHONY: all configure reconfigure build build-deterministic build-deterministic-nopy test test-nopy test-fast test-slow test-regression test-all test-golden-main test-golden-tag test-golden install clean venv nvfortran nvfortran-test nvfortran-test-nopy nvfortran-configure nvfortran-clean
Evidence
The .PHONY list includes venv but omits the newly-added venv-nopy target, while the target is
defined later in the file.

Makefile[17-18]
Makefile[101-106]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`venv-nopy` is a new Makefile target but it is not listed in `.PHONY`, so Make may treat it as a file target if `./venv-nopy` exists.

### Issue Context
This is a small robustness fix for the new venv workflow targets.

### Fix Focus Areas
- Makefile[17-18]

### Suggested change
Add `venv-nopy` to the `.PHONY:` declaration alongside `venv`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Comment on lines +49 to +58
if [ ! -f "${SCRIPT_DIR}/build/build.ninja" ]; then
echo " Configuring CMake ..."
cmake -S "$SCRIPT_DIR" -B"${SCRIPT_DIR}/build" -GNinja \
-DCMAKE_BUILD_TYPE=Release -DCMAKE_COLOR_DIAGNOSTICS=ON
fi
echo " Building Fortran library ..."
cmake --build "${SCRIPT_DIR}/build" --config Release

echo " Installing pysimple in editable mode ..."
pip install --no-build-isolation -e "$SCRIPT_DIR"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Stale cmake disables bindings 🐞 Bug ✓ Correctness

setup-venv.sh only configures CMake when build/build.ninja is missing, so an existing build/
configured with ENABLE_PYTHON_INTERFACE=OFF can be reused and pysimple won’t be built correctly.
This can lead to pip install -e . producing a package that fails to import because the python/
subdirectory is skipped by CMake.
Agent Prompt
### Issue description
`setup-venv.sh` reuses an existing `build/` directory if `build/build.ninja` exists, without ensuring `-DENABLE_PYTHON_INTERFACE=ON`. If a developer previously ran `make build-deterministic-nopy` (which configures `build/` with `ENABLE_PYTHON_INTERFACE=OFF`), the venv setup will reuse that cache and the Python bindings won’t be built/installed correctly.

### Issue Context
- `Makefile` has a `build-deterministic-nopy` target that configures `build/` with `-DENABLE_PYTHON_INTERFACE=OFF`.
- Top-level `CMakeLists.txt` only adds `python/` when `ENABLE_PYTHON_INTERFACE` is enabled.
- `pyproject.toml` sets scikit-build-core `build-dir = "build"`, so pip editable installs will interact with the same build directory.

### Fix Focus Areas
- setup-venv.sh[46-58]
- Makefile[73-85]
- pyproject.toml[13-19]

### Implementation direction
Do one of the following (preferred options first):
1. Use a dedicated build directory for the venv workflow (e.g. `build_venv/`), so it cannot conflict with `make build-deterministic-nopy`.
2. Always run an explicit reconfigure step when `BUILD_PYSIMPLE=1`, passing `-DENABLE_PYTHON_INTERFACE=ON` (and optionally `-DSIMPLE_ENABLE_PYTHON_TOOLS=ON`) even when `build/build.ninja` exists.
3. Detect a cached `CMakeCache.txt` with `ENABLE_PYTHON_INTERFACE:BOOL=OFF` and either error out with a clear message or automatically reconfigure/clean.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Add setup-venv.sh that creates .venv with all Python dependencies
(scipy, netCDF4, matplotlib, f90wrap, etc.) and builds pysimple
Fortran-Python bindings. Adds `make venv` and `make venv-nopy`
targets.

Also fixes:
- requirements.txt: use minimum version constraints instead of exact
  pins, which break on newer Python versions (e.g., 3.14) that lack
  prebuilt wheels for the pinned versions
- pyproject.toml: remove invalid [project.package-data] section that
  newer scikit-build-core (>=0.12) rejects as an unknown key
- .gitignore: add .venv/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant